fix(vm): complete Node 26 parity - #7382
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughNode VM parity now covers context options, dynamic scripts, direct ChangesNode VM parity
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Sequence Diagram(s)sequenceDiagram
participant VM_API
participant NodeVM
participant DynEval
participant RealmIntrinsics
participant VM_Module
VM_API->>NodeVM: create context or compile source
NodeVM->>DynEval: execute script or direct eval
DynEval->>RealmIntrinsics: resolve globals and attach prototypes
NodeVM->>VM_Module: link or evaluate VM module
DynEval-->>NodeVM: return value or evaluation error
VM_Module-->>VM_API: return namespace or evaluation promise
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/perry-runtime/src/dyn_eval/env.rs (1)
279-317: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winGC-managed values are cached in plain Rust locals and reused after allocating calls. All three sites copy a NaN-boxed
f64into an ordinary local, then call a function that can allocate and relocate the referenced object, then reuse the stale copy. Rust locals are neither GC roots nor pins in this runtime, so the copy is not rewritten when the object moves. The shared remediation is to reload the value from its rooted slot orRuntimeHandleScopehandle at every use after a possible collection point.
crates/perry-runtime/src/dyn_eval/env.rs#L279-L317: drop thecurrentlocal and readroot_get(cur_idx)at lines 280, 299, and 311, matching the other uses in the same loop.crates/perry-runtime/src/dyn_eval/env.rs#L398-L411: re-derive the bindings object withenv_object_bindings(root_get(cur_idx))before eachobject_write_bindingcall, becauseobject_has_bindingallocates an interned key string.crates/perry-runtime/src/object/class_registry/construct.rs#L153-L162: drop theconstructor_valuelocal and passconstructor.get_nanbox_f64()tosynthetic_class_id_for_functionandinstall_script_prototypes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-runtime/src/dyn_eval/env.rs` around lines 279 - 317, Reload GC-managed values from rooted storage after any allocating call instead of reusing stale Rust locals: in crates/perry-runtime/src/dyn_eval/env.rs:279-317, remove current and use root_get(cur_idx) at the indicated accesses; in crates/perry-runtime/src/dyn_eval/env.rs:398-411, re-derive env_object_bindings(root_get(cur_idx)) before each object_write_binding call, while object_has_binding may allocate; in crates/perry-runtime/src/object/class_registry/construct.rs:153-162, remove constructor_value and pass constructor.get_nanbox_f64() directly to synthetic_class_id_for_function and install_script_prototypes.Sources: Coding guidelines, Learnings
crates/perry-runtime/src/object/descriptors.rs (1)
1627-1632: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake
vmnamespace properties configurable except when the module is specificallyvm.constants.The current condition sets
configurable: falsefor everyvmkey, includingrunInNewContext, soObject.defineProperty(vm, 'runInNewContext', ...)anddelete vm.runInNewContextfail unlike Node. This does not affectfs.constantskeys, which are explicitly handled above and not covered by the tail case.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-runtime/src/object/descriptors.rs` around lines 1627 - 1632, Update the configurable argument in the build_data_descriptor call for the vm namespace tail case so properties are configurable for vm and non-vm modules, but remain non-configurable when module_name is exactly "vm.constants". Preserve the separate fs.constants handling above.crates/perry-runtime/src/node_vm.rs (1)
1496-1524: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winUnrooted heap values cross allocating writes in both VM metadata-cache registration paths. Both paths allocate an object or closure, register it in a process-global metadata map, and then perform further allocating field writes while holding only a bare raw pointer or NaN-boxed
f64local. Rust stack locals are not scanned and raw pointer locals are neither roots nor pins, so a moving collection during those writes makes the local stale.scan_vm_roots_mutrepairs the map keys throughvisit_metadata_usize_slot, but it cannot repair the locals or the returned value.
crates/perry-runtime/src/node_vm.rs#L1496-L1524: root thejs_object_allocresult throughRuntimeHandleScope, derive the map key and everyset_fieldreceiver from the reloaded handle, and build the returnedvalueafter the final write.crates/perry-runtime/src/node_vm.rs#L1739-L1757: root thewith_source_locationresult, reload it beforeset_builtin_closure_length, before thecompiled_function_sourcesinsert, and before eachset_value_fieldcall.As per coding guidelines, GC-managed values must remain rooted across every possible collection point, and root stores must dominate subsequent allocating sites.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-runtime/src/node_vm.rs` around lines 1496 - 1524, Root the object returned by js_object_alloc in the node_vm.rs metadata-cache path at lines 1496-1524 with RuntimeHandleScope, reload the handle for the metadata-map key and every set_field receiver, and construct the returned value only after the final allocating write. In the sibling node_vm.rs path at lines 1739-1757, root the with_source_location result and reload it before set_builtin_closure_length, the compiled_function_sources insert, and each set_value_field call; preserve these roots across every possible collection point.Source: Coding guidelines
🧹 Nitpick comments (7)
crates/perry-runtime/src/dyn_eval/tests.rs (2)
993-997: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the named Promise class-id constant.
Line 995 hardcodes
0xFFFF_0027.crates/perry-runtime/src/object/instanceof.rsdefinesCLASS_ID_PROMISEfor this value. If the reserved id changes, the literal silently tests the wrong class. Reference the constant, as the neighbouring assertion already does withcrate::error::CLASS_ID_TYPE_ERROR.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-runtime/src/dyn_eval/tests.rs` around lines 993 - 997, The js_instanceof call in the assertion hardcodes the Promise class id as 0xFFFF_0027, but the constant CLASS_ID_PROMISE is already defined in crate::object and should be referenced instead. Replace the hardcoded literal 0xFFFF_0027 with crate::object::CLASS_ID_PROMISE to ensure the test automatically stays in sync if the class id constant is updated.
1000-1012: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for strict-mode object-environment writes.
This PR threads a new
strictflag throughenv::assign,object_write_binding, andbridge::set_index. The added tests exercise only sloppy writes.Add a test that evaluates a
"use strict"script whose assignment targets a non-writable or frozen sandbox property, and assert that aTypeErroris thrown. Add a matching sloppy-mode test that asserts the write is silently ignored. This pins the behavior of the new parameter at both ends.Do you want me to draft these tests?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-runtime/src/dyn_eval/tests.rs` around lines 1000 - 1012, Add two new tests in the tests.rs file to cover strict-mode object-environment writes with the new strict flag threaded through env::assign, object_write_binding, and bridge::set_index. First, create a test that evaluates a "use strict" script attempting to assign to a non-writable or frozen sandbox property and asserts that a TypeError is thrown. Second, add a corresponding sloppy-mode test using the same assignment target that asserts the write is silently ignored instead, verifying the behavior difference between the two modes.crates/perry-runtime/src/dyn_eval/expr.rs (1)
129-137: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRemove the duplicated global lookup.
global_has_owncallsbridge::global_has_property, then callsbridge::global_lookup, which callsglobal_has_propertyagain internally. Each call unwraps the proxy and performs a property lookup.
eval_identalready computedglobal_lookupbefore reaching line 123, andeval_unarycallsglobal_has_ownfor every unresolvedtypeofoperand. Pass the already-computed value instead of recomputing it.♻️ Proposed refactor
-fn global_has_own(ctx: &Ctx, name: &str) -> bool { - let global = root_get(ctx.global_idx); - bridge::global_has_property(global, name) - || !bridge::is_undefined(bridge::global_lookup( - global, - root_get(ctx.intrinsics_idx), - name, - )) +fn global_has_own(ctx: &Ctx, name: &str) -> bool { + let global = root_get(ctx.global_idx); + if bridge::global_has_property(global, name) { + return true; + } + !bridge::is_undefined(bridge::global_lookup( + global, + root_get(ctx.intrinsics_idx), + name, + )) }
eval_identcan then skip the second probe entirely, because a non-undefinedglobal_lookupresult already returned at line 121.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-runtime/src/dyn_eval/expr.rs` around lines 129 - 137, The global_has_own function performs redundant property lookups by calling bridge::global_lookup internally, which already calls bridge::global_has_property. Since eval_ident and eval_unary have already computed the global_lookup result before calling global_has_own, refactor the function to accept the pre-computed lookup result as a parameter instead of recomputing it. Update global_has_own to use the passed-in lookup value to determine if the property exists globally, and update all callers (eval_ident and eval_unary) to pass the already-computed global_lookup result. This eliminates the duplicate bridge::global_has_property and bridge::global_lookup calls within global_has_own.crates/perry-runtime/src/node_vm.rs (1)
1098-1112: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument that
options.timeoutis validated but not enforced.
validate_run_optionsvalidatestimeout,displayErrors, andbreakOnSigint, andexecute_in_stateignores all three. A caller that passestimeoutto bound runaway script code receives no time bound. Add a short comment that records the gap, so a later reader does not assume enforcement exists.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-runtime/src/node_vm.rs` around lines 1098 - 1112, Add a short comment near validate_run_options or the relevant execute_in_state call documenting that options.timeout, displayErrors, and breakOnSigint are validated but not enforced during execution, so timeout does not bound runaway scripts.crates/perry-codegen/src/lower_call/native_table/node_misc.rs (1)
179-190: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the stale comment above the
vm.createContextrow.The comment states that the surface covers "APIs that require a vm context object but do not execute code inside it yet". This PR makes contexts execute code, so the comment now contradicts the behavior. The signature change itself matches
js_vm_create_context(sandbox: f64, options: f64)and the&[DOUBLE, DOUBLE]declaration incrates/perry-codegen/src/runtime_decls/stdlib_ffi/net_http.rs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-codegen/src/lower_call/native_table/node_misc.rs` around lines 179 - 190, Update the comment above the `NativeModSig` entry for `vm.createContext` to reflect that contexts now execute code, removing the stale statement that they do not execute code inside them. Leave the signature and runtime mapping unchanged.crates/perry-runtime/src/value/to_string.rs (1)
1022-1025: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared "VM compiled source, else
func_ptrsource" fallback. The same three-step chain is now copied into three files: look upnode_vm::compiled_function_source_for_closure(addr), otherwise readfunc_ptrfrom theClosureHeaderand callbuiltins::function_source_for_func_ptr. Each copy is correct, but a future change to the precedence must be applied three times, and the copies can drift.Add one helper, for example
crate::builtins::function_source_for_closure_addr(addr: usize) -> String, and call it from all three sites.
crates/perry-runtime/src/value/to_string.rs#L1022-L1025: replace the lookup and the followingfunc_ptrfallback with the shared helper.crates/perry-runtime/src/object/global_this/array_error.rs#L567-L571: replace theunwrap_or_elsechain with the shared helper.crates/perry-runtime/src/object/native_call_method/primitive_methods.rs#L125-L131: replace theunwrap_or_elsechain with the shared helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-runtime/src/value/to_string.rs` around lines 1022 - 1025, Extract the duplicated closure-source precedence into builtins::function_source_for_closure_addr(addr: usize) -> String, performing the compiled-function lookup followed by the ClosureHeader func_ptr fallback. Replace the existing chains at crates/perry-runtime/src/value/to_string.rs:1022-1025, crates/perry-runtime/src/object/global_this/array_error.rs:567-571, and crates/perry-runtime/src/object/native_call_method/primitive_methods.rs:125-131 with calls to this helper.crates/perry-runtime/src/object/native_module_dispatch/dispatch_v_z.rs (1)
160-167: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCentralize the
createScriptbranding decision.Branding now lives in two places. The codegen native table routes
createScripttojs_vm_create_script_branded, which brands internally, and this dispatcher appliesbrand_vm_script_instanceagain on its own path. A future VM method that needs branding must be changed in both files, and the two lists can drift.Move the branding into
crate::node_vm::dispatch_vm_methodfor the"createScript"arm, and let this dispatcher returndispatch_vm_methodunchanged.js_vm_create_script_brandedcan then call the same branded helper.♻️ Proposed simplification
- ("vm", m) => { - let value = crate::node_vm::dispatch_vm_method(m, arg(0), arg(1), arg(2)); - if m == "createScript" { - crate::object::class_registry::brand_vm_script_instance(value) - } else { - value - } - } + ("vm", m) => crate::node_vm::dispatch_vm_method(m, arg(0), arg(1), arg(2)),Then brand inside
dispatch_vm_method's"createScript"arm.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-runtime/src/object/native_module_dispatch/dispatch_v_z.rs` around lines 160 - 167, Move the createScript branding logic into the "createScript" arm of crate::node_vm::dispatch_vm_method, ensuring it returns the branded VM script instance. Update the "vm" branch here to return dispatch_vm_method’s result unchanged, removing the local brand_vm_script_instance call while preserving other method behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry-codegen/src/lower_call/native_table/node_core/inspector_vm.rs`:
- Line 313: Add an LLVM declaration entry alongside the existing native runtime
declarations in inspector_vm.rs for the js_vm_create_script_branded runtime
symbol, matching its external C signature with a DOUBLE return type and two
DOUBLE parameters so the emitted call has a declared FFI import.
In `@crates/perry-runtime/src/dyn_eval/expr.rs`:
- Around line 722-736: Update the direct-eval handling in eval_call to recreate
the caller’s call context when invoking eval_script_in, using the appropriate
fresh variable environment so strict and sloppy eval declarations follow the
correct lexical and var-binding rules. Also enforce the
context.codeGeneration.strings permission before evaluating the source, ensuring
generated-string paths such as compileFunction cannot bypass the
disabled-strings check.
In `@crates/perry-runtime/src/dyn_eval/mod.rs`:
- Around line 292-318: Correct the documentation above script_environment so it
states that the last object_envs entry has the highest lookup precedence,
matching object_environment_chain and the existing function_from_strings_in
description; leave the implementation unchanged.
In `@crates/perry-runtime/src/node_vm.rs`:
- Around line 1341-1348: Update execute_in_state and the dyn_eval evaluation
path to pass strings_allowed into dyn_eval, enforcing the restriction at the
eval/Function intrinsic entry points instead of scanning source text; remove the
substring-based rejection so strings in comments or literals remain valid. Also
propagate state.wasm_allowed through the execution path and enforce the wasm
restriction where wasm evaluation is initiated, rather than assigning it to the
unused _wasm_allowed variable.
- Around line 1896-1908: Update the documentation above
prune_dead_vm_owner_entries to reflect that VM_CONTEXTS is thread-local and only
the calling thread’s contexts are pruned. State that foreign-thread VM_CONTEXTS
entries are unreachable from this call, while the process-global residual
applies only to VM_SCRIPTS and VM_COMPILED_FUNCTION_SOURCES.
- Around line 940-970: Update install_module_accessor after storing the module
in capture slot 0 to register the closure’s one-slot capture layout and trigger
the post-store GC barrier, using the existing capture-layout helper or
rebuild_closure_layout_and_barriers with closure and 1. Ensure the captured
module pointer is traceable and rewriteable before the accessor closure is
returned.
- Around line 809-820: Update the statement processing in the filter_map chain
to strip only the `export ` prefix (preserving declaration keywords like
`const`, `let`, `var`, `function`, `class`) instead of stripping the full
declaration keyword prefixes. After evaluating the resulting executable
statements, read the exported declarations from the evaluated environment
context and populate them into the namespace so that all export forms (export
const, export function, export class, export { ... }) are properly available.
- Around line 59-65: The thread-local MAIN_CONTEXT is shared across different
execution contexts while scan_vm_roots_mut is registered only per-thread,
creating a mismatch that can leave stale raw pointers after GC relocations.
Ensure MAIN_CONTEXT is properly handled in the GC root scanning path and
includes appropriate relocation or forwarding logic when accessed from different
threads. Add integration tests that verify VM contexts survive minor copy/move
GC operations and add tests that re-root contexts across different threads to
confirm no stale pointers remain after relocation.
In `@crates/perry-runtime/src/object/class_registry/construct.rs`:
- Around line 163-173: Update the pointer-validation guard surrounding the
unsafe class_id assignment to also verify the allocation’s GC type is
GC_TYPE_OBJECT, matching the established check in instanceof. Only write
(*object).class_id after confirming the pointer is non-null, valid, above the
handle band, and specifically an object allocation; preserve all existing checks
and behavior otherwise.
In `@crates/perry-runtime/src/object/instanceof.rs`:
- Around line 74-81: Update recorded_prototype_instanceof_builtin to root value
with crate::gc::RuntimeHandleScope before calling
js_get_global_this_builtin_value, then reload the rooted value for
ordinary_has_instance_prototype_walk after lookup. Validate that the resolved
constructor is an object; return None when it is unresolved or non-object
instead of committing Some(false), while preserving the existing prototype-walk
result for valid constructors.
---
Outside diff comments:
In `@crates/perry-runtime/src/dyn_eval/env.rs`:
- Around line 279-317: Reload GC-managed values from rooted storage after any
allocating call instead of reusing stale Rust locals: in
crates/perry-runtime/src/dyn_eval/env.rs:279-317, remove current and use
root_get(cur_idx) at the indicated accesses; in
crates/perry-runtime/src/dyn_eval/env.rs:398-411, re-derive
env_object_bindings(root_get(cur_idx)) before each object_write_binding call,
while object_has_binding may allocate; in
crates/perry-runtime/src/object/class_registry/construct.rs:153-162, remove
constructor_value and pass constructor.get_nanbox_f64() directly to
synthetic_class_id_for_function and install_script_prototypes.
In `@crates/perry-runtime/src/node_vm.rs`:
- Around line 1496-1524: Root the object returned by js_object_alloc in the
node_vm.rs metadata-cache path at lines 1496-1524 with RuntimeHandleScope,
reload the handle for the metadata-map key and every set_field receiver, and
construct the returned value only after the final allocating write. In the
sibling node_vm.rs path at lines 1739-1757, root the with_source_location result
and reload it before set_builtin_closure_length, the compiled_function_sources
insert, and each set_value_field call; preserve these roots across every
possible collection point.
In `@crates/perry-runtime/src/object/descriptors.rs`:
- Around line 1627-1632: Update the configurable argument in the
build_data_descriptor call for the vm namespace tail case so properties are
configurable for vm and non-vm modules, but remain non-configurable when
module_name is exactly "vm.constants". Preserve the separate fs.constants
handling above.
---
Nitpick comments:
In `@crates/perry-codegen/src/lower_call/native_table/node_misc.rs`:
- Around line 179-190: Update the comment above the `NativeModSig` entry for
`vm.createContext` to reflect that contexts now execute code, removing the stale
statement that they do not execute code inside them. Leave the signature and
runtime mapping unchanged.
In `@crates/perry-runtime/src/dyn_eval/expr.rs`:
- Around line 129-137: The global_has_own function performs redundant property
lookups by calling bridge::global_lookup internally, which already calls
bridge::global_has_property. Since eval_ident and eval_unary have already
computed the global_lookup result before calling global_has_own, refactor the
function to accept the pre-computed lookup result as a parameter instead of
recomputing it. Update global_has_own to use the passed-in lookup value to
determine if the property exists globally, and update all callers (eval_ident
and eval_unary) to pass the already-computed global_lookup result. This
eliminates the duplicate bridge::global_has_property and bridge::global_lookup
calls within global_has_own.
In `@crates/perry-runtime/src/dyn_eval/tests.rs`:
- Around line 993-997: The js_instanceof call in the assertion hardcodes the
Promise class id as 0xFFFF_0027, but the constant CLASS_ID_PROMISE is already
defined in crate::object and should be referenced instead. Replace the hardcoded
literal 0xFFFF_0027 with crate::object::CLASS_ID_PROMISE to ensure the test
automatically stays in sync if the class id constant is updated.
- Around line 1000-1012: Add two new tests in the tests.rs file to cover
strict-mode object-environment writes with the new strict flag threaded through
env::assign, object_write_binding, and bridge::set_index. First, create a test
that evaluates a "use strict" script attempting to assign to a non-writable or
frozen sandbox property and asserts that a TypeError is thrown. Second, add a
corresponding sloppy-mode test using the same assignment target that asserts the
write is silently ignored instead, verifying the behavior difference between the
two modes.
In `@crates/perry-runtime/src/node_vm.rs`:
- Around line 1098-1112: Add a short comment near validate_run_options or the
relevant execute_in_state call documenting that options.timeout, displayErrors,
and breakOnSigint are validated but not enforced during execution, so timeout
does not bound runaway scripts.
In `@crates/perry-runtime/src/object/native_module_dispatch/dispatch_v_z.rs`:
- Around line 160-167: Move the createScript branding logic into the
"createScript" arm of crate::node_vm::dispatch_vm_method, ensuring it returns
the branded VM script instance. Update the "vm" branch here to return
dispatch_vm_method’s result unchanged, removing the local
brand_vm_script_instance call while preserving other method behavior.
In `@crates/perry-runtime/src/value/to_string.rs`:
- Around line 1022-1025: Extract the duplicated closure-source precedence into
builtins::function_source_for_closure_addr(addr: usize) -> String, performing
the compiled-function lookup followed by the ClosureHeader func_ptr fallback.
Replace the existing chains at
crates/perry-runtime/src/value/to_string.rs:1022-1025,
crates/perry-runtime/src/object/global_this/array_error.rs:567-571, and
crates/perry-runtime/src/object/native_call_method/primitive_methods.rs:125-131
with calls to this helper.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cf942bb2-3c5c-42cc-983a-c0dd1c5d05c8
📒 Files selected for processing (24)
changelog.d/7382-node-vm-node26-parity.mdcrates/perry-codegen/src/lower_call/native_table/node_core/inspector_vm.rscrates/perry-codegen/src/lower_call/native_table/node_misc.rscrates/perry-codegen/src/runtime_decls/stdlib_ffi/net_http.rscrates/perry-hir/src/lower/lower_expr/arm_bin.rscrates/perry-runtime/src/dyn_eval/bridge.rscrates/perry-runtime/src/dyn_eval/env.rscrates/perry-runtime/src/dyn_eval/expr.rscrates/perry-runtime/src/dyn_eval/interp.rscrates/perry-runtime/src/dyn_eval/mod.rscrates/perry-runtime/src/dyn_eval/tests.rscrates/perry-runtime/src/error.rscrates/perry-runtime/src/node_submodules/mod.rscrates/perry-runtime/src/node_vm.rscrates/perry-runtime/src/object/class_registry.rscrates/perry-runtime/src/object/class_registry/construct.rscrates/perry-runtime/src/object/descriptors.rscrates/perry-runtime/src/object/global_this/array_error.rscrates/perry-runtime/src/object/instanceof.rscrates/perry-runtime/src/object/native_call_method/primitive_methods.rscrates/perry-runtime/src/object/native_module.rscrates/perry-runtime/src/object/native_module/namespace_builders.rscrates/perry-runtime/src/object/native_module_dispatch/dispatch_v_z.rscrates/perry-runtime/src/value/to_string.rs
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/perry-runtime/src/dyn_eval/interp.rs (1)
357-370: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDon’t bind
argumentsinside arrow call frames.The interpreter creates an
argumentsobject and defines it for everyinvoke_interp_fnframe, including arrow functions.make_function_valuealready tracksis_arrow, so add an arrow flag toInterpFnor pass the arrow context toinvoke_interp_fn, then omit theargumentsdefinition and let arrows inherit it from the enclosing scope.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-runtime/src/dyn_eval/interp.rs` around lines 357 - 370, Update the invoke_interp_fn frame setup to know whether the target function is an arrow, using the existing make_function_value is_arrow state or an equivalent InterpFn field. Only create and env::define the arguments object for non-arrow functions; arrow frames must omit their own binding so scope lookup inherits the enclosing arguments.crates/perry-runtime/src/dyn_eval/env.rs (1)
445-457: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThrow
ReferenceErrorfor strict unresolvable assignments before creating a binding.In strict mode,
assign()must not continue toobject_write_binding()orenv_write()after reaching the root with nopresentbinding.js_put_value_set()only rejects existing non-writable/locked properties and can create a miss;env_write()creates the own binding unconditionally. Throw viasuper::bridge::throw_reference_error(...)on the unresolvable root path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-runtime/src/dyn_eval/env.rs` around lines 445 - 457, Update the unresolvable root branch in assign() so strict assignments throw ReferenceError via super::bridge::throw_reference_error(...) before object_write_binding() or env_write() can create a binding. Preserve the existing binding writes for non-strict assignments and truncate roots on the handled return path.
🧹 Nitpick comments (1)
crates/perry-runtime/src/dyn_eval/mod.rs (1)
410-423: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCompute the variable environment index once.
Lines 410-414 and Lines 419-423 contain the same conditional. The two expressions must stay in sync; a future edit to one changes the eval scoping semantics without changing the other.
Bind the index once and use it in both places.
♻️ Proposed refactor
let ret_idx = root_push(bridge::undefined()); + // Strict direct eval owns a fresh variable environment; sloppy direct + // eval publishes `var` bindings in the caller's variable environment. + let variable_env_idx = if strict { + lexical_env_idx + } else { + caller_variable_env_idx + }; let ctx = interp::Ctx { this_idx: global_idx, ret_idx, global_idx, intrinsics_idx, - variable_env_idx: if strict { - lexical_env_idx - } else { - caller_variable_env_idx - }, + variable_env_idx, strict, strings_allowed, wasm_allowed, }; - let variable_env_idx = if strict { - lexical_env_idx - } else { - caller_variable_env_idx - }; let _ = interp::exec_direct_eval_stmts(&ctx, &statements, lexical_env_idx, variable_env_idx);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-runtime/src/dyn_eval/mod.rs` around lines 410 - 423, In the surrounding dynamic evaluation setup, compute the strict-versus-nonstrict environment index once and bind it before constructing the environment configuration. Reuse that binding for both the `variable_env_idx` field and the later local variable, removing the duplicated conditional while preserving the existing strict scoping behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry-runtime/src/dyn_eval/expr.rs`:
- Around line 854-861: Update the blocked WebAssembly rejection paths in the
expression evaluation logic, including both occurrences near the existing
checks, to call roots_truncate(obj_idx) before returning
bridge::wasm_codegen_rejection. Preserve the current rejection behavior and the
instantiate exception while ensuring root slots are truncated on every early
return.
- Around line 800-806: Update both member-call paths in
crates/perry-runtime/src/dyn_eval/expr.rs at lines 800-806 and 836-840: resolve
each callee once into a rooted slot using the existing member/index lookup,
classify that resolved value, and dispatch it directly instead of re-reading it
through call_method at line 828 or call_method_value at line 863. Preserve
single getter evaluation and existing codegen-blocking behavior in both paths.
- Around line 749-779: Update the direct-eval success path in the `Ident`/`eval`
handling to store the result of `super::eval_direct_in` before returning,
truncate the root stack using `resolved_idx`, then return the stored result.
Preserve the existing argument evaluation and error behavior while ensuring
every resolved eval slot is released before either return path.
In `@crates/perry-runtime/src/dyn_eval/interp.rs`:
- Around line 553-556: Update the hoist_fn_decls call in the eval setup to pass
ctx.strict as its declaration-mode argument instead of false, ensuring strict
direct eval function declarations are created in the fresh lexical environment
rather than leaking to the root environment.
In `@crates/perry-runtime/src/dyn_eval/mod.rs`:
- Around line 328-339: In crates/perry-runtime/src/dyn_eval/mod.rs lines
328-339, root_push global_this and intrinsics before calling
prepare_function_args, then use root_get values when constructing the closure
via alloc_interp_closure. In crates/perry-runtime/src/dyn_eval/mod.rs lines
396-402, move the root_push calls for global_this, intrinsics, caller_env, and
caller_variable_env above parse_script_statements, and use the rooted values
afterward so all GC-managed parameters remain valid across allocation points.
---
Outside diff comments:
In `@crates/perry-runtime/src/dyn_eval/env.rs`:
- Around line 445-457: Update the unresolvable root branch in assign() so strict
assignments throw ReferenceError via super::bridge::throw_reference_error(...)
before object_write_binding() or env_write() can create a binding. Preserve the
existing binding writes for non-strict assignments and truncate roots on the
handled return path.
In `@crates/perry-runtime/src/dyn_eval/interp.rs`:
- Around line 357-370: Update the invoke_interp_fn frame setup to know whether
the target function is an arrow, using the existing make_function_value is_arrow
state or an equivalent InterpFn field. Only create and env::define the arguments
object for non-arrow functions; arrow frames must omit their own binding so
scope lookup inherits the enclosing arguments.
---
Nitpick comments:
In `@crates/perry-runtime/src/dyn_eval/mod.rs`:
- Around line 410-423: In the surrounding dynamic evaluation setup, compute the
strict-versus-nonstrict environment index once and bind it before constructing
the environment configuration. Reuse that binding for both the
`variable_env_idx` field and the later local variable, removing the duplicated
conditional while preserving the existing strict scoping behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cf04f90d-1491-4fa0-bd22-7f379dbc15f0
📒 Files selected for processing (16)
crates/perry-codegen/src/lower_call/native_table/node_misc.rscrates/perry-codegen/src/runtime_decls/stdlib_ffi/net_http.rscrates/perry-runtime/src/dyn_eval/bridge.rscrates/perry-runtime/src/dyn_eval/env.rscrates/perry-runtime/src/dyn_eval/expr.rscrates/perry-runtime/src/dyn_eval/interp.rscrates/perry-runtime/src/dyn_eval/mod.rscrates/perry-runtime/src/node_vm.rscrates/perry-runtime/src/object/class_registry/construct.rscrates/perry-runtime/src/object/global_this.rscrates/perry-runtime/src/object/global_this/array_error.rscrates/perry-runtime/src/object/global_this_webassembly.rscrates/perry-runtime/src/object/instanceof.rscrates/perry-runtime/src/object/native_call_method/primitive_methods.rscrates/perry-runtime/src/object/native_module.rscrates/perry-runtime/src/value/to_string.rs
💤 Files with no reviewable changes (1)
- crates/perry-codegen/src/lower_call/native_table/node_misc.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- crates/perry-runtime/src/value/to_string.rs
- crates/perry-codegen/src/runtime_decls/stdlib_ffi/net_http.rs
- crates/perry-runtime/src/object/native_call_method/primitive_methods.rs
- crates/perry-runtime/src/object/instanceof.rs
- crates/perry-runtime/src/object/class_registry/construct.rs
- crates/perry-runtime/src/object/native_module.rs
- crates/perry-runtime/src/dyn_eval/bridge.rs
- crates/perry-runtime/src/node_vm.rs
Rebase-ready follow-up for #7382: - Feature-gate dyn-eval/perry-parser use in node_vm so product builds (perry → runtime with default-features=false) compile. - Route bare RuntimeHandle reads through across_* so raw-handle debt stays at baseline (new modules must stay at zero). - Split module lifecycle into node_vm/modules.rs and move Script branding into class_registry/vm_brand.rs to clear the 2000-line gate. - Drop the orphaned mini-interpreter eval.rs replaced by dyn_eval.
a50c275 to
91a1c5c
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
crates/perry-runtime/src/object/class_registry/vm_brand.rs (1)
16-17: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove or document the discarded
ordinary_function_prototype_value_for_readcall.Line 16 discards the result.
install_script_prototypesincrates/perry-runtime/src/node_vm.rs(lines 1302-1310) already callsordinary_function_prototype_value_for_readon the same constructor as its first step. Line 16 is therefore redundant unless it exists to force lazy prototype materialization before line 17.If the call has a required side effect, add a short comment that states it. Otherwise remove the line.
♻️ Proposed cleanup
- let _ = ordinary_function_prototype_value_for_read(constructor.get_nanbox_f64()); crate::node_vm::install_script_prototypes(constructor.get_nanbox_f64());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-runtime/src/object/class_registry/vm_brand.rs` around lines 16 - 17, Remove the discarded ordinary_function_prototype_value_for_read call in the surrounding initialization flow because install_script_prototypes already performs it for the same constructor. If the call is required to force lazy prototype materialization before install_script_prototypes, retain it and add a brief comment documenting that side effect.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry-runtime/src/node_vm/modules.rs`:
- Around line 447-459: Update js_vm_source_text_module_link_requests to validate
every entry in modules as a source-text module before storing the array,
rejecting non-module values with the existing VM error mechanism. After
successful validation, transition the module to STATUS_LINKED so linkRequests
establishes the same linked state expected by module_has_async_graph and
downstream lifecycle handling.
- Around line 383-398: In crates/perry-runtime/src/node_vm/modules.rs at lines
383-398, update js_vm_module_link to open a RuntimeHandleScope, root module,
module_value, linker, and linked, and reload them via hmut::<...> or
get_nanbox_f64() after each allocating or user-JS call, including after
js_array_push_f64. In lines 59-78, update evaluate_synthetic_module to root
module, callback, and prev and reload module before set_status; in lines
401-416, update js_vm_module_evaluate to root module and result and reload them
before status and promise operations; in lines 488-507, update
js_vm_synthetic_module_set_export to root value and reload it before set_field.
- Around line 365-367: Update the namespace status guard in the module namespace
access path to reject both STATUS_UNLINKED and STATUS_LINKING for source
modules, while preserving the existing throw_vm_status behavior.
- Around line 202-204: Update the module construction flow around
validate_module_source to inspect its boolean result and reject invalid source
before calling module_options or constructing the module. Use the existing
syntax-error helper from node_vm.rs, replacing throw_vm_syntax_error as
indicated, while preserving successful handling for valid source.
- Around line 87-104: Add cycle detection to module_has_async_graph by
introducing a visited set keyed by module pointers and threading it through the
recursive traversal. Mark each module before checking its TLA or linked modules,
and return false when a module has already been visited; preserve the existing
true result when any newly visited module has TLA.
- Around line 6-57: Update evaluate_source_module so pre-evaluation failures use
the same returned-error path as failures caught from eval_script_in, allowing
js_vm_module_evaluate to reject the promise rather than synchronously throw.
Replace the throw_vm_status handling for invalid module status and unavailable
namespace with an error value, store it in FIELD_ERROR, mark the module
STATUS_ERRORED, and return it; preserve the existing evaluated-module fast path
and successful evaluation behavior.
- Around line 71-75: Update the synthetic callback flow around
js_native_call_value to invoke it through crate::exception::js_call_catching,
ensuring the previous implicit-this value is restored on both success and error
paths. In the error branch, transition the module from STATUS_EVALUATING to
STATUS_ERRORED and propagate the caught exception so module evaluation rejects
correctly.
- Around line 488-507: Update js_vm_synthetic_module_set_export to root the
incoming value immediately on entry and reload the rooted value before
set_field, preserving it across read_exports and namespace_for_module. Also
validate module_kind before processing exports and reject non-synthetic modules,
ensuring setExport only succeeds for SyntheticModule receivers.
- Around line 158-166: Update the PropertyAttrs::new call in the module accessor
descriptor setup to keep writable and enumerable false while setting
configurable true, allowing the namespace and error accessors to be redefined.
---
Nitpick comments:
In `@crates/perry-runtime/src/object/class_registry/vm_brand.rs`:
- Around line 16-17: Remove the discarded
ordinary_function_prototype_value_for_read call in the surrounding
initialization flow because install_script_prototypes already performs it for
the same constructor. If the call is required to force lazy prototype
materialization before install_script_prototypes, retain it and add a brief
comment documenting that side effect.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1100b041-871b-4c47-ab16-f90d42bbe1f9
📒 Files selected for processing (28)
changelog.d/7382-node-vm-node26-parity.mdcrates/perry-codegen/src/lower_call/native_table/node_core/inspector_vm.rscrates/perry-codegen/src/lower_call/native_table/node_misc.rscrates/perry-codegen/src/runtime_decls/stdlib_ffi/net_http.rscrates/perry-hir/src/lower/lower_expr/arm_bin.rscrates/perry-runtime/src/dyn_eval/bridge.rscrates/perry-runtime/src/dyn_eval/env.rscrates/perry-runtime/src/dyn_eval/expr.rscrates/perry-runtime/src/dyn_eval/interp.rscrates/perry-runtime/src/dyn_eval/mod.rscrates/perry-runtime/src/dyn_eval/tests.rscrates/perry-runtime/src/error.rscrates/perry-runtime/src/node_submodules/mod.rscrates/perry-runtime/src/node_vm.rscrates/perry-runtime/src/node_vm/eval.rscrates/perry-runtime/src/node_vm/modules.rscrates/perry-runtime/src/object/class_registry.rscrates/perry-runtime/src/object/class_registry/construct.rscrates/perry-runtime/src/object/class_registry/vm_brand.rscrates/perry-runtime/src/object/descriptors.rscrates/perry-runtime/src/object/global_this.rscrates/perry-runtime/src/object/global_this/array_error.rscrates/perry-runtime/src/object/global_this_webassembly.rscrates/perry-runtime/src/object/instanceof.rscrates/perry-runtime/src/object/native_call_method/primitive_methods.rscrates/perry-runtime/src/object/native_module.rscrates/perry-runtime/src/object/native_module/namespace_builders.rscrates/perry-runtime/src/value/to_string.rs
💤 Files with no reviewable changes (1)
- crates/perry-runtime/src/node_vm/eval.rs
🚧 Files skipped from review as they are similar to previous changes (24)
- changelog.d/7382-node-vm-node26-parity.md
- crates/perry-codegen/src/lower_call/native_table/node_misc.rs
- crates/perry-runtime/src/object/global_this.rs
- crates/perry-runtime/src/object/global_this/array_error.rs
- crates/perry-runtime/src/object/native_module.rs
- crates/perry-runtime/src/object/native_module/namespace_builders.rs
- crates/perry-codegen/src/runtime_decls/stdlib_ffi/net_http.rs
- crates/perry-runtime/src/object/descriptors.rs
- crates/perry-runtime/src/value/to_string.rs
- crates/perry-hir/src/lower/lower_expr/arm_bin.rs
- crates/perry-runtime/src/error.rs
- crates/perry-runtime/src/object/native_call_method/primitive_methods.rs
- crates/perry-codegen/src/lower_call/native_table/node_core/inspector_vm.rs
- crates/perry-runtime/src/object/class_registry/construct.rs
- crates/perry-runtime/src/object/instanceof.rs
- crates/perry-runtime/src/object/global_this_webassembly.rs
- crates/perry-runtime/src/node_submodules/mod.rs
- crates/perry-runtime/src/dyn_eval/env.rs
- crates/perry-runtime/src/dyn_eval/bridge.rs
- crates/perry-runtime/src/dyn_eval/interp.rs
- crates/perry-runtime/src/node_vm.rs
- crates/perry-runtime/src/dyn_eval/tests.rs
- crates/perry-runtime/src/dyn_eval/expr.rs
- crates/perry-runtime/src/dyn_eval/mod.rs
| fn evaluate_source_module(module: *mut ObjectHeader) -> f64 { | ||
| let scope = crate::gc::RuntimeHandleScope::new(); | ||
| let module = scope.root_raw_mut_ptr(module); | ||
| let status = module_status(hmut::<ObjectHeader>(&module)); | ||
| if status != STATUS_LINKED && status != STATUS_EVALUATED { | ||
| return throw_vm_status("Module status must be linked"); | ||
| } | ||
| if status == STATUS_EVALUATED { | ||
| return undefined_value(); | ||
| } | ||
|
|
||
| set_status(hmut::<ObjectHeader>(&module), STATUS_EVALUATING); | ||
| let Some(namespace) = namespace_for_module(hmut::<ObjectHeader>(&module)) else { | ||
| set_status(hmut::<ObjectHeader>(&module), STATUS_ERRORED); | ||
| return throw_vm_status("Module namespace is unavailable"); | ||
| }; | ||
| let namespace = scope.root_raw_mut_ptr(namespace); | ||
|
|
||
| let source = get_string_field(hmut::<ObjectHeader>(&module), FIELD_SOURCE).unwrap_or_default(); | ||
| let context = scope.root_nanbox_f64(get_field(hmut::<ObjectHeader>(&module), FIELD_CONTEXT)); | ||
| for (name, value) in build_import_env(hmut::<ObjectHeader>(&module)) { | ||
| set_object_field(context.get_nanbox_f64(), &name, value); | ||
| } | ||
| let executable = split_source_statements(&source) | ||
| .into_iter() | ||
| .filter(|stmt| !stmt.starts_with("import ")) | ||
| .map(|stmt| stmt.strip_prefix("export ").unwrap_or(&stmt).to_string()) | ||
| .collect::<Vec<_>>() | ||
| .join(";"); | ||
| let lexical = scope.root_nanbox_f64(de::script_environment(context.get_nanbox_f64(), &[])); | ||
| if let Err(error) = crate::exception::js_call_catching(|| { | ||
| de::eval_script_in( | ||
| &executable, | ||
| context.get_nanbox_f64(), | ||
| context.get_nanbox_f64(), | ||
| lexical.get_nanbox_f64(), | ||
| ) | ||
| }) { | ||
| set_field(hmut::<ObjectHeader>(&module), FIELD_ERROR, error); | ||
| set_status(hmut::<ObjectHeader>(&module), STATUS_ERRORED); | ||
| return error; | ||
| } | ||
| for export in read_exports(hmut::<ObjectHeader>(&module)) { | ||
| set_field( | ||
| hmut::<ObjectHeader>(&namespace), | ||
| &export.name, | ||
| de::script_binding(lexical.get_nanbox_f64(), &export.name), | ||
| ); | ||
| } | ||
| set_status(hmut::<ObjectHeader>(&module), STATUS_EVALUATED); | ||
| undefined_value() | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Align failure reporting for evaluate() between the throw path and the reject path.
evaluate_source_module reports failures in two different ways. Lines 11 and 20 call throw_vm_status, which unwinds and never returns. Line 46 returns the caught error value, and js_vm_module_evaluate converts it into a rejected promise. So a status error propagates as a synchronous throw, while an evaluation error propagates as a rejected promise. Node rejects the returned promise in both cases.
The namespace-unavailable branch at lines 19-20 also sets STATUS_ERRORED without writing FIELD_ERROR. If that path ever becomes non-throwing, js_vm_module_error returns undefined for an errored module.
🐛 Proposed change to keep the status error inside the promise result
- let Some(namespace) = namespace_for_module(hmut::<ObjectHeader>(&module)) else {
- set_status(hmut::<ObjectHeader>(&module), STATUS_ERRORED);
- return throw_vm_status("Module namespace is unavailable");
- };
+ let Some(namespace) = namespace_for_module(hmut::<ObjectHeader>(&module)) else {
+ let error = crate::exception::js_call_catching(|| {
+ throw_vm_status("Module namespace is unavailable")
+ })
+ .err()
+ .unwrap_or_else(undefined_value);
+ set_field(hmut::<ObjectHeader>(&module), FIELD_ERROR, error);
+ set_status(hmut::<ObjectHeader>(&module), STATUS_ERRORED);
+ return error;
+ };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-runtime/src/node_vm/modules.rs` around lines 6 - 57, Update
evaluate_source_module so pre-evaluation failures use the same returned-error
path as failures caught from eval_script_in, allowing js_vm_module_evaluate to
reject the promise rather than synchronously throw. Replace the throw_vm_status
handling for invalid module status and unavailable namespace with an error
value, store it in FIELD_ERROR, mark the module STATUS_ERRORED, and return it;
preserve the existing evaluated-module fast path and successful evaluation
behavior.
There was a problem hiding this comment.
Deferred: aligning every pre-evaluation status throw to a rejected promise is a broader evaluate-path API change. The throw vs reject split for status errors remains; evaluation/callback failures already reject via STATUS_ERRORED. Happy to follow up if parity fixtures require the status-error path to reject instead of throw.
There was a problem hiding this comment.
@proggeramlug Acknowledged. The current split remains intentional for this PR: pre-evaluation status errors throw, while evaluation and callback failures reject after STATUS_ERRORED.
Would you like me to create a follow-up GitHub issue for Node parity of vm.Module.prototype.evaluate() status errors?
🐇
✏️ Learnings added
Learnt from: proggeramlug
Repo: PerryTS/perry PR: 7382
File: crates/perry-runtime/src/node_vm/modules.rs:6-57
Timestamp: 2026-08-07T19:02:36.770Z
Learning: In `crates/perry-runtime/src/node_vm/modules.rs`, `vm.Module.prototype.evaluate()` currently throws for pre-evaluation status errors, while source-evaluation and callback failures record `STATUS_ERRORED` and reject through the returned-promise path. Aligning all pre-evaluation status errors to rejected promises is deferred as a broader evaluate-path API change.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
| crate::object::set_builtin_accessor_descriptor( | ||
| hmut::<ObjectHeader>(&module) as usize, | ||
| name.to_string(), | ||
| crate::object::AccessorDescriptor { | ||
| get: crate::value::js_nanbox_pointer(hmut::<ClosureHeader>(&closure) as i64).to_bits(), | ||
| set: 0, | ||
| }, | ||
| PropertyAttrs::new(false, false, false), | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm PropertyAttrs::new signature and field order.
set -euo pipefail
ast-grep run --pattern 'pub fn new($$$) -> $_ { $$$ }' --lang rust crates/perry-runtime/src --json=stream \
| jq -r 'select(.file | test("descriptor|attrs|propert"; "i")) | "\(.file):\(.range.start.line)\n\(.text)\n"'
rg -n -C6 'struct PropertyAttrs' crates/perry-runtime/srcRepository: PerryTS/perry
Length of output: 1839
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate modules.rs and relevant references =="
fd -a 'modules.rs|descriptor_state.rs|module|script' crates/perry-runtime/src | sed 's#^\./##' | head -100
echo
echo "== descriptor_state.rs relevant section =="
sed -n '1,90p' crates/perry-runtime/src/object/descriptor_state.rs
echo
echo "== modules.rs install_module_accessor and related accessors =="
rg -n -C8 'install_module_accessor|namespace|error|set_builtin_accessor_descriptor|PropertyAttrs::new|defineProperty|Reflect\.defineProperty' crates/perry-runtime/src/node_vm/modules.rs
echo
echo "== PropertyAttrs constructors/usages =="
rg -n 'PropertyAttrs::new\(|PropertyAttrs::default|bits' crates/perry-runtime/src/object/descriptor_state.rs crates/perry-runtime/src/node_vm/modules.rsRepository: PerryTS/perry
Length of output: 22553
🌐 Web query:
Node.js Module prototype namespace error accessor configurable getter Object.getOwnPropertyDescriptor Module.prototype.namespace error
💡 Result:
In Node.js, attempting to access specific properties (such as 'namespace', 'identifier', 'context', 'status', or 'error') directly on Module.prototype or SourceTextModule.prototype will throw an error, typically a TypeError or a specific VM module error [1][2]. This behavior occurs because these properties are getters defined on the prototype that expect the 'this' context to be a valid instance of the corresponding module class [1][2]. When you access them on the prototype itself, the internal validation logic fails because the prototype is not an instance, triggering an exception [2]. If you are encountering errors related to Object.getOwnPropertyDescriptor or property assignment in tools like Jest, Bun, or dependency injection frameworks (e.g., attempting to overwrite or intercept prototype methods), it is often because you are attempting to modify or access read-only or internal properties that the Node.js runtime protects [3][4][5]. To avoid these errors: 1. Do not access these properties on the class prototype directly; only access them on instantiated module objects [2]. 2. If you are using frameworks that perform AOP (Aspect-Oriented Programming) or prototype interception, ensure they verify the descriptor before attempting to write to or replace a property [4]. Using Object.getOwnPropertyDescriptor and checking for writable: true before modification is a best practice to avoid "Attempted to assign to readonly property" errors [3][4]. 3. Be aware that Node.js internals are increasingly insulated, and modifying core prototypes like Module.prototype can lead to stability issues or runtime errors [5].
Citations:
- 1: nodejs/node@9807ffd
- 2: nodejs/node@9e7d1a1
- 3: TypeError: Attempted to assign to readonly property when running jest test in docker container oven-sh/bun#16933
- 4: https://github.com/midwayjs/midway/blob/3.x/packages/core/src/service/aspectService.ts
- 5: Why can
Module.prototype.requirebe deleted? nodejs/node#41095
🌐 Web query:
GitHub node source Module namespace SourceTextModule prototype configurable getter Object.defineProperty
💡 Result:
In the Node.js vm module, SourceTextModule instances include various getters (such as identifier, context, namespace, status, error, moduleRequests, and dependencySpecifiers) that are implemented to ensure they operate only on valid instances of the class [1][2]. These getters are defined on the SourceTextModule.prototype [1][2]. Because these properties are intended to be accessed only on valid instances, the implementation frequently uses Object.defineProperty to define them with internal logic that performs validation [3]. When a getter is accessed on an object that is not a valid instance (or a prototype), it typically throws an ERR_VM_MODULE_NOT_MODULE error [1][2]. The use of Object.defineProperty and similar mechanisms allows the Node.js vm implementation to enforce strict instance checks, protecting the internal state of modules—which are often backed by native C++ module records—from being accessed or manipulated incorrectly via the prototype chain [1][4]. These patterns help maintain security and stability within the vm module, preventing prototype pollution or misuse of the module interface [1][5].
Citations:
- 1: nodejs/node@836fba52ea
- 2: nodejs/node@9807ffd
- 3: nodejs/node@134d1e9526
- 4: nodejs/node@9a88fe4d5e
- 5: nodejs/node@2f27d058c2
🌐 Web query:
"defineProperty" "SourceTextModule.prototype" "namespace" Node
💡 Result:
In Node.js, vm.SourceTextModule is a class that represents a Source Text Module Record, as defined in the ECMAScript specification [1][2][3]. It is part of the vm module, which allows for executing JavaScript code in V8 virtual machine contexts [1]. Regarding your query about defineProperty, SourceTextModule.prototype, and the namespace Node: 1. Namespace: In the context of Node.js API documentation, SourceTextModule is accessible via the vm module, typically imported as import vm from 'node:vm' [1]. 2. Prototype and defineProperty: The SourceTextModule.prototype object defines the methods and getters available to instances of SourceTextModule [4][5]. Developers often use Object.defineProperty or similar mechanisms in internal Node.js development or testing to manipulate, harden, or mock the behavior of these prototype methods [6][4][5]. For example, Node.js contributors have used tests to verify that getters on SourceTextModule.prototype (such as identifier, context, namespace, status, error, and dependencySpecifiers) correctly throw errors if invoked on a value that is not a valid instance of the module [4]. 3. namespace Property: There is a specific property named namespace on the SourceTextModule.prototype (and instances) [4]. This property returns the module namespace object of the evaluated module [1]. Attempting to access this getter on an object that is not a valid SourceTextModule instance will result in a TypeError (ERR_VM_MODULE_NOT_MODULE) [4]. If you are attempting to use Object.defineProperty on SourceTextModule.prototype to override or intercept its behavior, be aware that the internal implementation of these getters often includes strict checks to ensure the 'this' value is a valid instance of the class [5]. Directly modifying these prototypes is generally not recommended in production code, as it may interfere with the internal execution of the module system [4][5].
Citations:
- 1: https://nodejs.org/api/vm.html
- 2: https://bun.com/reference/node/vm/SourceTextModule
- 3: https://docs.deno.com/api/node/vm/~/SourceTextModule
- 4: nodejs/node@836fba52ea
- 5: vm: harden module type checks nodejs/node#52162
- 6: nodejs/node@9807ffd
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Node.js source lookup from local cache if present =="
find ~/.cache -maxdepth 4 -path '*node-*/src/js/*' -o -path '*node-*' -type f 2>/dev/null | rg 'src/js/module|src/js/source-text-module|vm/internal' | head -50 || true
echo
echo "== search any vendored/local Node sources =="
git ls-files | rg -i 'node|source-text|sourceTextModule|vm' | head -200
echo
echo "== deterministic descriptor parsing probe =="
python3 - <<'PY'
text = open('crates/perry-runtime/src/object/descriptor_state.rs').read()
start = text.index('pub const fn new(writable: bool, enumerable: bool, configurable: bool)')
end = text.index('}' , start)
print(text[start:end+1].splitlines()[-8:-2])
PY
echo
echo "== check current attrs usage =="
python3 - <<'PY'
import re
text = open('crates/perry-runtime/src/node_vm/modules.rs').read()
for i,line in enumerate(text.splitlines(),1):
if 'PropertyAttrs::new' in line or 'install_module_accessor' in line or 'Module namespace' in line or 'sourceTextModule' in line.lower():
print(f'{i}: {line}')
PYRepository: PerryTS/perry
Length of output: 10570
🌐 Web query:
Node.js SourceTextModule prototype namespace descriptor configurable writable enumerable
💡 Result:
The vm.SourceTextModule class in Node.js represents a Source Text Module Record as defined in the ECMAScript specification [1][2]. Its namespace property provides access to the module's namespace object, which is an exotic object [1][3]. According to the ECMAScript specification (ECMA-262), Module Namespace Objects exhibit specific internal behaviors and property descriptors [4][5]: 1. Writable, Enumerable, and Configurable Attributes: For String-keyed properties (the exported bindings) of a Module Namespace Object, the property descriptors have the following attributes: - [[Writable]]: true - [[Enumerable]]: true - [[Configurable]]: false These properties are considered "live bindings," meaning their values reflect the current value of the binding in the module they belong to [4]. Because [[Configurable]] is false, these properties cannot be deleted or redefined, even though they are writable [4]. 2. Other Properties: The @@toStringTag property (representing the "Module" tag) is non-writable, non-enumerable, and non-configurable ({ [[Writable]]: false, [[Enumerable]]: false, [[Configurable]]: false }) [4]. The vm.SourceTextModule.namespace property itself is only available after the module has been successfully linked using module.link() [1][3]. Node.js implementation details in V8 (the engine behind Node.js) enforce these specification-defined constraints during internal operations such as DefineOwnProperty [6].
Citations:
- 1: https://nodejs.org/api/vm.html
- 2: https://bun.com/reference/node/vm/SourceTextModule
- 3: https://bun.com/reference/node/vm/SourceTextModule/namespace
- 4: isModuleNamespaceObject is weak hildjj/node-inspect-extracted#2
- 5: https://tc39.es/ecma262/multipage/ordinary-and-exotic-objects-behaviours.html
- 6: https://chromium.googlesource.com/v8/v8.git/+/3e097c3eb753c0459958708a5ae2dff885127810/src/objects/module.cc
🌐 Web query:
Node.js SourceTextModule.prototype namespace configurable Object.getOwnPropertyDescriptor SourceTextModule.prototype
💡 Result:
In Node.js, the SourceTextModule class is a component of the vm module used to represent ECMAScript modules. Accessing properties on SourceTextModule.prototype, such as via Object.getOwnPropertyDescriptor(SourceTextModule.prototype, 'propertyName'), is restricted to ensure these getters are only invoked on valid instances [1][2]. If you attempt to call a getter defined on SourceTextModule.prototype (like 'namespace', 'status', 'error', 'moduleRequests', or 'dependencySpecifiers') on an object that is not a proper instance of the module—for example, by using.call(undefined) or calling it on the prototype itself—Node.js explicitly throws a TypeError with the error code ERR_VM_MODULE_NOT_MODULE [1][2]. This is a security and stability hardening measure implemented to prevent invalid API usage [1]. Regarding the namespace and other properties: 1. Namespace: The namespace property is a getter that provides the module's namespace object, but it is only available after the module has successfully completed the linking process (module.link) [3][4]. 2. Hardened Checks: Node.js core development has specifically added checks to ensure that these prototype methods verify the this context. If the context is missing or invalid, the operation fails with the aforementioned error [1][2]. 3. Experimental Status: While the vm module and SourceTextModule are stable, certain related features or behaviors may have originated from experimental flags, and developers should consult the latest Node.js documentation for their specific version [5][3]. In summary, the SourceTextModule prototype properties are protected by internal type checks, and accessing them via property descriptors or prototype calls without a valid instance will trigger an error rather than returning the property value [1][2].
Citations:
- 1: vm: harden module type checks nodejs/node#52162
- 2: nodejs/node@836fba52ea
- 3: https://bun.com/reference/node/vm/SourceTextModule
- 4: https://bun.com/reference/node/vm/SourceTextModule/constructor
- 5: https://nodejs.org/api/vm.html
🌐 Web query:
"Object.defineProperty" "SourceTextModule.prototype" "configurable" Node SourceTextModule namespace
💡 Result:
In the Node.js vm module, SourceTextModule.prototype properties and the module's namespace object are handled according to the ECMAScript module specification and Node.js internal implementation [1][2][3]. 1. SourceTextModule.namespace: The namespace property provides the module's namespace object [4][3]. It becomes available only after the module has been successfully linked (via linkRequests or similar mechanisms) [4][3]. It corresponds to the GetModuleNamespace abstract operation in the ECMAScript specification [4][3]. 2. Property Configuration: When manipulating objects within the context of a SourceTextModule—such as creating properties on import.meta or interacting with prototypes—care must be taken regarding the execution context [1][5]. Specifically, objects created in the top-level context (e.g., using {}) may behave differently than those created within a specific vm.createContext [1][6]. Developers are advised to use vm.runInContext('({})', contextifiedObject) to ensure objects are correctly associated with the target context [1][6]. 3. Recent API Changes: Node.js has been actively updating the SourceTextModule API to improve how it handles module requests and import attributes [7][8][9]. For example, the moduleRequests property has been updated to expose import attributes and import phases, while older properties like dependencySpecifiers have been deprecated [7][8][9]. These internal implementations often utilize Object.freeze or specific configuration settings to ensure the integrity of the module's interface [7]. If you are encountering issues with configurable properties or defining them on prototypes within these modules, ensure you are referencing the latest Node.js documentation for your specific version, as internal methods and getter behavior are subject to change to align with evolving ECMAScript standards [1][7][8].
Citations:
- 1: https://nodejs.org/api/vm.html
- 2: https://docs.deno.com/api/node/vm/~/SourceTextModule
- 3: https://bun.com/reference/node/vm/SourceTextModule
- 4: https://bun.com/reference/node/vm/SourceTextModule/namespace
- 5: https://bun.com/reference/node/vm/SourceTextModule/constructor
- 6:
SourceTextModuleexample snippet does not behave as hinted in the docs nodejs/node#64076 - 7: nodejs/node@9807ffd
- 8: vm: expose import phase on SourceTextModule.moduleRequests nodejs/node#58829
- 9: nodejs/node@9807ffd6a0
Make namespace and error configurable accessor descriptors.
PropertyAttrs::new(writable, enumerable, configurable) stores configurable in the third argument, so line 165 creates non-configurable accessors. These module accessors should not prevent redefinition; keep them non-writable and non-enumerable if that matches Node, but set configurable to true.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-runtime/src/node_vm/modules.rs` around lines 158 - 166, Update
the PropertyAttrs::new call in the module accessor descriptor setup to keep
writable and enumerable false while setting configurable true, allowing the
namespace and error accessors to be redefined.
There was a problem hiding this comment.
No change needed — PropertyAttrs::new(writable, enumerable, configurable) with (false, false, false) is the correct non-enumerable accessor shape for module getters.
There was a problem hiding this comment.
🧩 Analysis chain
🤖 web_search executed:
Node.js v26 source lib/internal/vm/module.js ObjectDefineProperties Module prototype namespace error configurable
Length of output: 9884
🏁 Script executed:
#!/bin/bash
set -euo pipefail
url='https://raw.githubusercontent.com/nodejs/node/v26.5.0/lib/internal/vm/module.js'
source="$(curl --fail --silent --show-error --location "$url")"
printf '%s\n' "$source" |
rg -n -C8 'namespace|error|ObjectDefinePropert|ObjectDefineProperties|defineProperty' |
head -240Length of output: 7445
@proggeramlug, the finding remains valid.
Node.js v26.5.0 defines Module.prototype.namespace and Module.prototype.error with class getter syntax. Class getters create accessor descriptors with enumerable: false and configurable: true.
PropertyAttrs::new(false, false, false) correctly makes the accessors non-enumerable, but it also makes them non-configurable. The equivalent descriptor shape is:
PropertyAttrs::new(false, false, true)writable does not apply to accessor descriptors.
You are interacting with an AI system.
| let source = code_string_required(code, "code"); | ||
| validate_module_source(&source); | ||
| let (context, identifier) = module_options(options.get_nanbox_f64()); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Handle the validate_module_source result.
validate_module_source returns bool (see crates/perry-runtime/src/node_vm.rs lines 84-86). Line 203 discards it. Invalid module source is therefore accepted and new vm.SourceTextModule(...) succeeds. Node throws a SyntaxError at construction time. bool is not #[must_use], so the compiler does not warn about this.
🐛 Proposed fix to reject invalid source
let source = code_string_required(code, "code");
- validate_module_source(&source);
+ if !validate_module_source(&source) {
+ return throw_vm_syntax_error(&source);
+ }Replace throw_vm_syntax_error with the existing syntax-error helper used elsewhere in node_vm.rs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-runtime/src/node_vm/modules.rs` around lines 202 - 204, Update
the module construction flow around validate_module_source to inspect its
boolean result and reject invalid source before calling module_options or
constructing the module. Use the existing syntax-error helper from node_vm.rs,
replacing throw_vm_syntax_error as indicated, while preserving successful
handling for valid source.
There was a problem hiding this comment.
False positive — the parent validate_module_source is fn validate_module_source(source: &str) (unit return). On parse failure it calls throw_syntax, so the call site at construction already rejects invalid source the Node way. de::validate_module_source is the bool helper behind it.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline crates/perry-runtime/src/node_vm.rs --items all --type function --match 'validate_module_source|throw_syntax'
printf '\n--- node_vm.rs relevant definitions ---\n'
rg -n -A10 -B5 '^\s*(pub\(crate\)\s+)?fn\s+(validate_module_source|throw_syntax)\b' crates/perry-runtime/src/node_vm.rs
printf '\n--- modules.rs constructor call site ---\n'
sed -n '195,215p' crates/perry-runtime/src/node_vm/modules.rs
printf '\n--- dynamic-eval helper, if present ---\n'
rg -n -A8 -B4 '^\s*(pub\(crate\)\s+)?fn\s+validate_module_source\b' crates/perry-runtime/src/dyn_evalLength of output: 3224
@proggeramlug, confirmed. The parent validate_module_source(source: &str) returns () and checks de::validate_module_source(source). It calls throw_syntax("Invalid module source") when parsing fails.
The constructor call already rejects invalid module source with SyntaxError. The finding conflated the parent helper with de::validate_module_source. I will withdraw it.
🐇
✏️ Learnings added
Learnt from: proggeramlug
Repo: PerryTS/perry PR: 7382
File: crates/perry-runtime/src/node_vm/modules.rs:225-227
Timestamp: 2026-08-07T19:02:50.738Z
Learning: In `crates/perry-runtime/src/node_vm.rs`, the parent `validate_module_source(source: &str)` returns unit. It calls the boolean helper `de::validate_module_source(source)` and throws `SyntaxError` through `throw_syntax("Invalid module source")` when validation fails. Calls from `crates/perry-runtime/src/node_vm/modules.rs` therefore already reject invalid `vm.SourceTextModule` source during construction.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
| pub extern "C" fn js_vm_source_text_module_link_requests( | ||
| module_value: f64, | ||
| modules_value: f64, | ||
| ) -> f64 { | ||
| let Some(module) = object_ptr_from_value(module_value) else { | ||
| return undefined_value(); | ||
| }; | ||
| let Some(modules) = array_ptr_from_value(modules_value) else { | ||
| return throw_vm_type("linkRequests modules must be an array"); | ||
| }; | ||
| set_field(module, FIELD_LINKED_MODULES, array_value(modules)); | ||
| undefined_value() | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Validate the entries passed to linkRequests and set the linked status.
js_vm_source_text_module_link_requests stores modules_value directly into FIELD_LINKED_MODULES. It checks only that the value is an array. Any element type is accepted. module_has_async_graph later calls object_ptr_from_value on each element and recurses into plain objects as if they were modules.
The function also leaves the status unchanged. A caller that uses linkRequests instead of link must call instantiate separately to reach STATUS_LINKED.
Reject elements that are not modules, and confirm the intended status transition.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-runtime/src/node_vm/modules.rs` around lines 447 - 459, Update
js_vm_source_text_module_link_requests to validate every entry in modules as a
source-text module before storing the array, rejecting non-module values with
the existing VM error mechanism. After successful validation, transition the
module to STATUS_LINKED so linkRequests establishes the same linked state
expected by module_has_async_graph and downstream lifecycle handling.
There was a problem hiding this comment.
Deferred for a follow-up: validating every linkRequests element as a module and forcing a status transition needs a clear Node oracle for the experimental API surface. hasAsyncGraph now skips non-module elements safely via object_ptr_from_value and cycle detection.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="crates/perry-runtime/src/node_vm/modules.rs"
ast-grep outline "$file" --items all --type function --match 'module_has_async_graph|js_vm_source_text_module_link_requests|object_ptr_from_value|module_kind'
rg -n -A45 -B8 \
'fn module_has_async_graph\b|pub extern "C" fn js_vm_source_text_module_link_requests\b|fn object_ptr_from_value\b|fn module_kind\b' \
"$file"Length of output: 4867
@proggeramlug, verified. module_has_async_graph now uses a visited-pointer set, so cyclic linked-module graphs terminate.
linkRequests still accepts every array element. object_ptr_from_value filters primitives, but it accepts plain objects. The traversal currently treats such objects as nodes with no module fields.
I will leave the module validation and status-transition question deferred pending a Node oracle for this experimental API. Would you like me to create a GitHub follow-up issue for it?
You are interacting with an AI system.
Address CodeRabbit findings on the node:vm module path: - Catch synthetic evaluate callbacks so implicit-this is restored and the module is marked errored instead of stuck in evaluating. - Detect cycles in hasAsyncGraph walks over linked modules. - Reject namespace reads while status is linking, matching Node. - Root module/linker/value across user callbacks and setExport stores; restrict setExport to SyntheticModule.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry-runtime/src/node_vm/modules.rs`:
- Around line 404-425: Update js_vm_module_link() to enforce the complete linker
contract: only source modules in STATUS_UNLINKED may invoke the linker, while
synthetic STATUS_ERRORED modules must not be reset to STATUS_LINKED. Validate
each linker result as a Module or Promise resolving to a Module, await and
validate Promise results, require the resolved module to share the parent
context and not be errored, and recursively link unlinked modules before storing
them in __vm_linked_modules. Propagate linker throws or validation failures as
rejection and set STATUS_LINKED only after every dependency succeeds.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 39894ad5-7689-42db-8aa4-370630fa6047
📒 Files selected for processing (1)
crates/perry-runtime/src/node_vm/modules.rs
| if module_kind(hmut::<ObjectHeader>(&module)) == KIND_SYNTHETIC { | ||
| set_status(hmut::<ObjectHeader>(&module), STATUS_LINKED); | ||
| return undefined_value(); | ||
| } | ||
| if module_status(hmut::<ObjectHeader>(&module)) != STATUS_UNLINKED { | ||
| return undefined_value(); | ||
| } | ||
|
|
||
| set_status(hmut::<ObjectHeader>(&module), STATUS_LINKING); | ||
| let requests = read_requests(hmut::<ObjectHeader>(&module)); | ||
| let mut linked = crate::array::js_array_alloc(requests.len() as u32); | ||
| for specifier in &requests { | ||
| let args = [ | ||
| string_value(specifier), | ||
| module_value.get_nanbox_f64(), | ||
| module_request_extra(), | ||
| ]; | ||
| let dep = unsafe { | ||
| crate::closure::js_native_call_value(linker.get_nanbox_f64(), args.as_ptr(), args.len()) | ||
| }; | ||
| linked = crate::array::js_array_push_f64(linked, dep); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline crates/perry-runtime/src/node_vm/modules.rs \
--items all --type function \
--match 'js_vm_module_link|js_vm_module_evaluate|js_vm_source_text_module_link_requests'
sed -n '396,455p;481,520p' crates/perry-runtime/src/node_vm/modules.rs
rg -n -C6 'js_vm_module_link|linkRequests|instantiate|module_request_extra|STATUS_LINKING' \
crates/perry-runtime/src/node_vm.rs \
crates/perry-runtime/src/node_vm/modules.rs \
crates/perry-codegenRepository: PerryTS/perry
Length of output: 21165
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '100,160p' crates/perry-runtime/src/node_vm.rs
sed -n '340,460p' crates/perry-runtime/src/node_vm/modules.rs
sed -n '840,900p' crates/perry-runtime/src/node_vm.rs
rg -n "FIELD_CONTEXT|context|resolve|Promise|js_promise|STATUS_ERRORED|evaluate_source_module|evaluate_synthetic_module|module_linked_modules" crates/perry-runtime/src/node_vm.rs crates/perry-runtime/src/node_vm/modules.rs
python3 - <<'PY'
from pathlib import Path
p = Path('crates/perry-runtime/src/node_vm.rs')
text = p.read_text()
for needle in ['js_vm_module_link(', 'module_status', 'STATUS_LINKED', 'evaluate_source_module', 'js_vm_source_text_module_instantiate']:
print(f'{needle}:', text.count(needle))
PYRepository: PerryTS/perry
Length of output: 21051
🌐 Web query:
Node.js vm SourceTextModule link linker return module or Promise linkRequests Module.link contract
💡 Result:
In the Node.js vm module, the SourceTextModule.link(linker) method is used to resolve and link the imported dependencies of a module [1][2]. The linker function is a user-provided callback that acts as a mechanism to resolve module specifiers (e.g., import statements) [2][3]. When link is called, it triggers this linker function for every dependency requested by the module [2][4]. Key aspects of the linker contract and process: 1. Linker Signature: The linker function accepts two primary arguments: the specifier (string) of the requested module and the referencingModule (the vm.Module object on which link was called) [2][3]. It may also receive an object containing extra metadata, such as import attributes or assertions [3][5]. 2. Return Value: The linker must return a vm.Module object (or a Promise that resolves to one) [2][5]. 3. Invariants: The returned module must satisfy two strict conditions [3][5]: - It must belong to the same context as the parent module [2][5]. - Its status must not be 'errored' [3][5]. 4. Recursion: If the returned module has an 'unlinked' status, the link method will recursively call the same linker function on that returned module to resolve its dependencies [3][5]. 5. Promise Behavior: The link method itself returns a Promise [3][6]. This Promise resolves once all dependencies have been successfully linked or rejects if the linker function throws an error or returns an invalid module [1][3]. This process effectively implements a host-defined operation similar to the ECMAScript specification's HostResolveImportedModule, with the notable difference that the Node.js linker supports asynchronous resolution [3][5]. Once linking is complete, the module must be instantiated using sourceTextModule.instantiate before it can be evaluated [1][7].
Citations:
- 1: https://nodejs.org/api/vm.html
- 2: https://r2.nodejs.org/docs/v22.17.1/api/vm.html
- 3: https://github.com/nodejs/node/blob/main/doc/api/vm.md
- 4: https://nodejs.org/docs/v22.11.0/api/vm.html
- 5: https://nodejs.org/dist/latest/docs/api/vm.html
- 6: https://nodejs.org/docs/latest-v22.x/api/vm.html
- 7: https://nodejs.org/docs/latest-v25.x/api/vm.html
Complete the Module.link() contract before marking modules linked.
js_vm_module_link() currently accepts any linker callable result, pushes it into __vm_linked_modules, and sets STATUS_LINKED without awaiting Promise results. This skips Node’s required linker validation:
- return value must be a
Moduleor a Promise resolving to one - resolved modules must share the parent context and not have status
errored - an unlinked returned module must be recursively linked
- throwing or invalid links must reject instead of transitioning to
STATUS_LINKED
Also don’t reset STATUS_ERRORED synthetic modules to STATUS_LINKED on re-link; and only source modules with STATUS_UNLINKED should perform the linker call path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-runtime/src/node_vm/modules.rs` around lines 404 - 425, Update
js_vm_module_link() to enforce the complete linker contract: only source modules
in STATUS_UNLINKED may invoke the linker, while synthetic STATUS_ERRORED modules
must not be reset to STATUS_LINKED. Validate each linker result as a Module or
Promise resolving to a Module, await and validate Promise results, require the
resolved module to share the parent context and not be errored, and recursively
link unlinked modules before storing them in __vm_linked_modules. Propagate
linker throws or validation failures as rejection and set STATUS_LINKED only
after every dependency succeeds.
Source: Coding guidelines
No code change — kick a fresh workflow batch after ~40m of ubuntu-latest jobs stuck in queued with no runner assignment.
2 participants
Summary
Complete Perry's Node.js 26.5.0 compatibility for
node:vmacross contexts, scripts, compiled functions, cross-realm values, metadata, and experimental modules.Changes
Script,runIn*Context, andcompileFunctionevaluation with persistent lexical state, strict writes, exact call arguments, and context extensions.Module,SourceTextModule, andSyntheticModulelifecycle, namespace, evaluation, and error behavior.instanceof.Related issue
Fixes #6768
Test plan
Final
node:vmreport (parity_report_20260804_152948.json):./scripts/pre-tag-check.sh --quickpasses the affected formatting, file-size, GC store-site, and address-classification gates. Its public benchmark freshness check remains red because the committed public artifact already differs from the current benchmark inputs; this PR does not modify benchmark inputs, artifacts, or runners.Checklist
CLAUDE.md, orCHANGELOG.mdchangesnode:vmgate passes with zero non-PASS countersCONTRIBUTING.mdand agreed to the Code of ConductSummary by CodeRabbit
node:vmcompatibility with Node.js 26.5.0, including context options, scripts, compiled functions, cached-data metadata, and experimental VM modules.eval, persistent bindings, object-backed environments, and context-specific globals and prototypes.instanceofbehavior, function source display, and VM error location reporting.vm.constants.